Initial support for remote reporters #5884
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WojciechMazur
commented
Nov 28, 2023
Comment on lines
16
to
18
| operation SendReportEvent { | ||
| input: ReportEvent | ||
| } |
There was a problem hiding this comment.
For the review of model the not-yet removed smithy schema can be used. The Java API has been generated based on that.
In perfect world we would be able to generate Java code from this schema, but there is no smithy codegen available for Java (only Kotlin and Scala has implemented codegens)
WojciechMazur
force-pushed
the
telemetry/initial-support-for-remote-reporting
branch
2 times, most recently
from
a152f62
to
98f0397
Compare
kasiaMarek
reviewed
Dec 22, 2023
mtags-shared/src/main/scala-2.12/scala/meta/internal/jdk/CollectionConverters.scala
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/ReportContext.scala
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/SourceCodeTransformer.scala
Outdated
Show resolved
Hide resolved
telemetry-interfaces/src/main/java/scala/meta/internal/telemetry/MetalsLspContext.java
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/TelemetryLevel.scala
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/SourceCodeTransformer.scala
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/SourceCodeSanitizer.scala
Outdated
Show resolved
Hide resolved
| val fillInLength = | ||
| originalSymbol.length() - prefix.length() - suffix.length() | ||
| val prefixTail = | ||
| if (fillInLength < 0) prefix.tail.take(-fillInLength) |
There was a problem hiding this comment.
Suggested change
| if (fillInLength < 0) prefix.tail.take(-fillInLength) | |
| if (fillInLength < 0) prefix.tail.dropRight(-fillInLength) |
Also if someone has like 100 names and uses new three letter one, this will produce a 4 letter substitution.
tgodzik
requested changes
Jan 9, 2024
build.sbt
Outdated
| @@ -270,9 +284,10 @@ lazy val mtagsShared = project | |||
| "org.lz4" % "lz4-java" % "1.8.0", | |||
| "com.google.protobuf" % "protobuf-java" % "3.25.1", | |||
| "io.get-coursier" % "interface" % V.coursierInterfaces, | |||
| "com.lihaoyi" %% "requests" % V.requests, | |||
There was a problem hiding this comment.
@rochala Will this not be a problem for the compiler?
There was a problem hiding this comment.
This will be a problem and has to be changed. We can't add any Scala dependencies directly to the compiler. This can be easily changed tho, so remote report context is implemented in metals, and passed to the PC via implicit. We don't need to implement it directly in the PC
metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/telemetry/conversion/package.scala
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/LoggerAccess.scala
Outdated
Show resolved
Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/TelemetryLevel.scala
Outdated
Show resolved
Hide resolved
telemetry-interfaces/src/main/java/scala/meta/internal/telemetry/ServiceEndpoint.java
Outdated
Show resolved
Hide resolved
telemetry-interfaces/src/main/java/scala/meta/internal/telemetry/UnknownProducerContext.java
Outdated
Show resolved
Hide resolved
tests/unit/src/test/scala/tests/telemetry/TelemetryReporterSuite.scala
Outdated
Show resolved
Hide resolved
WojciechMazur
force-pushed
the
telemetry/initial-support-for-remote-reporting
branch
from
99e29c3
to
8f0a4c9
Compare
rochala
force-pushed
the
telemetry/initial-support-for-remote-reporting
branch
from
449d618
to
17b6df4
Compare
Resolve conflicts in ReportContext. Refactor sanitzer to dedicated class Fix compilation based on CI results
…ontained in reports Fix cross-compilation problems
Fix source code sanitizer markdown snipets detection on Windows Run scalafix for test sources
…r with additional reports
…d throttling, deduplicate sent reports
rochala
force-pushed
the
telemetry/initial-support-for-remote-reporting
branch
from
1dba1b3
to
cca5308
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to provide a capability of collecting crash reports using a remote reporter.
We introduce a new project
telemetry-interfacesallowing to define a structured API for connecting between Metals components (MetalsLspService,ScalaPresentationCompiler) and a HTTP REST server aggregating the reports. API is defined using Java along with provided Gson codecs to ensure correct serialisation. Separate model would allow evolve the schema in the future.Initial prototype involved usage of Smithy4s for generation a Scala idiomatic model and to ensure usage of the same schema between metals client and remote server. However, this approach would introduce number of new dependencies to project, and would require to cross publish for each of the targets. The original smithy schema is temporary still available in the branch for a quick lookup of the model.
New
ReportContextimplementations:MirrorReportContext- duplexing of reporting, allowing to send reports to multiple reporters, eg. to file usingStdReportContextand remotlyRemoteTelemetryReportContext- a reporter using HTTP client to communicate with server aggregating reportsA dedicated server for remote telemetry was created in the separate repository. Temporaly it was installed in the Scala Open Community Build environment and available under
https://scala3.westeurope.cloudapp.azure.com/telemetryTODO:
Report.textshould be available in the Remot Reports - the local work with ScalaPresentationComiler suggests it can contain a full source code of failing to compile sources#solved:
textcontains important context required to reproduce issues. To allow for keeping orignal source code we introduce a SourceCodeSanitizer - it would try to parse sources using Scalameta/Dotty parser and replace all names with stable obfuscated symbols not containing any domain/author/organization information. Same applies to String literals which would be redacted out. In case if parsing/transformation of source code ast would fail no original sources would be reported.#solved:
UserConfiguration was updated to contain the
telemetryLevelspecified by the user, can be set using client config (Allow to pass telemetry level in user config metals-vscode#1441) or via Metals properties-D-Dmetals.telemetry-level={off, crash, error, all}